Summary

• extract named-backup discovery out of lib/storage.ts into a dedicated helper module
• keep the existing backup discovery and sorting behavior intact while continuing the storage split in small slices

What Changed

• added lib/storage/named-backups.ts with collectNamedBackups(...) and the NamedBackupSummary type
• updated lib/storage.ts to call the extracted helper and re-export the summary type expected by the current public surface
• preserved the current named-backup behavior through the storage test suite

Validation

• npm run test -- test/storage.test.ts
• npm run lint
• npm run typecheck
• npm run build

Risk and Rollback

• Risk level: low
• Rollback plan: revert 45cda79 to restore the inline named-backup discovery implementation

Additional Notes

• this continues the storage split after the path-state extraction in the same isolated worktree and stacked review flow

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

clean slice extraction — collectNamedBackups and NamedBackupSummary are moved from lib/storage.ts into a dedicated lib/storage/named-backups.ts helper, and AccountIdentityRef/toAccountIdentityRef are promoted to public exports in lib/storage/identity.ts. behavior is fully preserved and the existing storage-last-backup.test.ts integration suite validates the key scenarios end-to-end.

• lib/storage/named-backups.ts — new module with CollectNamedBackupsDeps interface enabling dependency injection for readDir, stat, loadAccountsFromPath, and an optional logDebug; logic is identical to what was in storage.ts
• lib/storage.ts — drops the inline implementation, delegates to collectNamedBackups with real node:fs/promises deps, and re-exports NamedBackupSummary to preserve the public surface
• lib/storage/identity.ts — AccountIdentityRef and toAccountIdentityRef made export so storage.ts can import them without re-declaring
• p2 — readDir type is overly broad: typeof import("node:fs").promises.readdir carries every overload; a narrower single-signature interface matching the one actual call would be easier to stub in unit tests
• p2 — no dedicated unit tests for the DI surface: storage-last-backup.test.ts covers behavior via the real fs through getNamedBackups(), but no test exercises collectNamedBackups with injected mocks directly — worth a follow-up test/storage/named-backups.test.ts to stay above the 80% coverage threshold and to cover the logDebug-optional and unknown-error-rethrow branches

Confidence Score: 4/5

• safe to merge — pure extraction with no logic change; two p2 follow-ups remain but neither blocks the happy path
• behavior is identical to pre-refactor, existing integration tests cover the main scenarios, typecheck/lint/build all pass. score stays at 4 rather than 5 because the DI interface was introduced specifically for testability but no unit tests against it were added, and the broad readDir type will make future mocking friction-y.
• lib/storage/named-backups.ts — readDir type breadth and missing unit tests for the DI surface

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["getNamedBackups()"] -->|injects real fs deps| B["collectNamedBackups(storagePath, deps)"]
    B --> C["deps.readDir(backupRoot, {withFileTypes:true})"]
    C -->|ENOENT| D["return []"]
    C -->|other error| E["throw"]
    C -->|entries| F["filter: isFile + .json"]
    F --> G["deps.stat(candidatePath) — statsBefore"]
    G --> H["deps.loadAccountsFromPath(candidatePath)"]
    H -->|null or 0 accounts| I["skip entry"]
    H -->|valid accounts| J["deps.stat(candidatePath) — statsAfter"]
    J -->|mtime changed| K["deps.logDebug? stale mtime warning"]
    J --> L["push to candidates[]"]
    G -->|throws| M["deps.logDebug? skip + continue"]
    L --> N["sort by mtimeMs desc, then fileName asc"]
    N --> O["return NamedBackupSummary[]"]

This is a comment left during a code review.
Path: lib/storage/named-backups.ts
Line: 12

Comment:
**overly broad `readDir` type makes mocking harder**

`typeof import("node:fs").promises.readdir` captures every overload (returns `string[]`, `Buffer[]`, or `Dirent[]` depending on options). this makes the type hard to satisfy in a unit-test stub and means TypeScript has to resolve overloads through the interface property at every call site. a narrower signature matching exactly what `collectNamedBackups` uses would be cleaner:

```suggestion
	readDir: (
		path: string,
		options: { withFileTypes: true; encoding: BufferEncoding },
	) => Promise<{ isFile(): boolean; name: string }[]>;
```

this is a better fit for the DI pattern and makes future unit tests trivial to write.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: lib/storage/named-backups.ts
Line: 20-22

Comment:
**no unit tests cover the DI surface**

the DI (`CollectNamedBackupsDeps`) was introduced specifically so `collectNamedBackups` can be tested in isolation without touching the real filesystem. `storage-last-backup.test.ts` exercises the behavior via `getNamedBackups()` end-to-end (real fs), which is good, but none of the unit tests inject mock deps directly against `collectNamedBackups`.

missing vitest coverage for injected paths:
- `readDir` returning an unknown `code` error (should re-throw)
- `logDebug` is omitted (optional) — skipped-entry logging is silently dropped
- `stat` throwing mid-loop — currently caught and swallowed via `logDebug`

a companion `test/storage/named-backups.test.ts` using stub deps would close this gap without real fs I/O and keep coverage above the 80% threshold required by the project.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (2): Last reviewed commit: "Merge main into refactor/pr3-storage-nam..." | Re-trigger Greptile}